correctness/clarification: "Calculates a request's checksum" and "If a user supplies a checksum via an HTTP header no calculation will be done." conflict with each other

naming: requestChecksumAlgorithm

Postfix -> Suffix

style: can be restructured as an extension function HttpRequest.checksumHeader(logger: Logger): String?

and naming: userProvidedChecksumHeader

docs: "Model sourced flag"

Why was this branch added? The spec says we should delay checksum calculation until the body is consumed:

Where possible, SDKs MUST defer this calculation and validation until the payload is actually consumed by the user i.e. a payload must not be read twice.

toHashingBody doesn't support Bytes bodies. I decided to calculate the checksum in memory instead of adding support for Bytes to toHashingBody. It should be safe if the request is not being streamed but I think you're right that we're not following the spec

toHashingBody doesn't support Bytes bodies

Does it need to? The previous implementation never seemed to need it

I think it does, the spec has some unit tests with non-streaming bodies

Discussed offline and we'll be removing support for HttpBody.Bytes from FlexibleChecksumsResponseInterceptor because HTTP bodies are never bytes. Source

I think removing support for HttpBody.Bytes is a bad idea. Our current implementations of OkHttp and CRT engines do not return bytes, that's true, but future implementations or other engines wrapped by users may. In addition, interceptors may be used to rewrite response bodies by streaming them into memory, processing them, and then rewriting the body in a new form (possibly HttpBody.Bytes). I think we need to ensure each type of HttpBody is handled correctly.

Why was this test removed?

We're no longer setting the checksum header via execution context

Why was this test removed?

This should be covered under ChecksumConfigTest > ResponseChecksumValidation but I added it back

docs: create -> calculate

correctness: missing tests for when requestChecksumRequired = false and requestChecksumCalculation = HttpChecksumConfigOption.WHEN_REQUIRED. Same for response validation

The request test cases are already here. I think the response validation unit tests need one more to be exhaustive.

Those tests in ChecksumConfigTest.kt aren't validating interceptor behavior like the tests in this file do.

Nit: "asynchronously" → "during transmission"

Nit: "synchronously" → "before transmission"

Nit: Seems unnecessary to declare forcedToCalculateChecksum, defaultChecksumAlgorithm, and defaultChecksumAlgorithmHeaderPostfix as fields only to use them in one if branch. Could this just be:

?: if (requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED) {
    checksumHeader.append("crc32")
    Crc32()
} else

Style: These log messages which talk about the user feel awkward. Log messages are for users so it seems strange to mention the user in the third person. I'd suggest rewording these to be more about data/actions and less about the actors involved (e.g., "Found a provided checksum in the request, skipping checksum calculation").

Nit: We've lost the log message that we're calculating a checksum asynchronously during transmission. I'd suggest moving your "Calculating checksum using '$checksumAlgorithm'" message into the if/else clauses, tweaking each to mention whether the checksum is being calculated before or during transmission.

Nit: When this logic is made S3 specific, we shouldn't assert on the part number fitting inside a certain range. Just "-(\d)+$" should suffice.

Nit: Please add tests for HttpChecksumConfigOption.WHEN_REQUIRED. Applies to response interceptors as well.

The request test cases are already here. I think the response validation unit tests need one more to be exhaustive.

Question: Do we still use HttpOperationContext.ChecksumAlgorithm or can it be deprecated?

I think it's still used for MD5 checksums in the httpChecksumRequiredTrait

Update on this, it's no longer used anywhere so we can deprecate/remove it

Question: Why did we delete this test?

This should be covered under ChecksumConfigTest > ResponseChecksumValidation but I added it back

Nit: "or if this is set" → "or if the service offers optional checksums"

FYI I know we talked about disabling some tests that are failing in Smithy 1.53.0, but these are disabling the entire test suite, which we don't want to do

This has never been a problem before, what changed?

There was a refactor in AWS SDK Kotlin to make the codegen tests KMP, which led to the JVM CI checks failing because we've been using JVM 17. I set the JVM version in the tests to 1.8 and the toList function used above is only available since JVM 16

Ok that makes sense. You should be able to use shapes(T::class.java).collect(Collectors.toList()) instead of hacking around with the Kotlin toList

nit/docs: Explain more about how it "handles" the trait

The "meat" of the integration is the middleware and both of them have their own Kdocs. It feels like docs overkill to also add a summary of what each middleware is doing here

question/correctness: Can/should we store this as a HashFunction rather than a String?

We could, I think we would have to initialize the HashFunction in the context and its seems slightly worse than what we have right now. Thoughts?

correctness: This seems like the wrong place to define this function

Where would be better?

I would have said HttpChecksumRequiredInterceptor but I see it's also used by FlexibleChecksumsRequestInterceptor. I think it's fine here.

style: make it an extension val

internal val ProtocolRequestInterceptorContext<Any, HttpRequest>.defaultChecksumAlgorithmName: String?
    get() = executionContext.getOrNull(HttpOperationContext.DefaultChecksumAlgorithm)

These used to be private, now they are @InternalApi public, meaning they are included in our API dumps and part of backwards compatibility. Was this move necessary?

Oops, I moved this here because I was going to add support for streaming checksums in the HttpCheksumsRequired interceptor

correctness: Relying on this::class.simpleName seems unsafe.

Better solution:

@InternalApi
public val HashFunction.isSupportedForFlexibleChecksums: Boolean 
    get() = when (this) {
        is Crc32, is Crc32c, is Sha1, is Sha256 -> true
        else -> false
    }

Yeah agree, that's what we had before but we would have to keep track of what checksum algorithms are supported for flexible checksums in two places. In HashFunction.isSupportedForFlexibleChecksums & algorithmsSupportedForFlexibleChecksums.

I made a tradeoff here, it seems more risky to possibly miss updating what algorithms we support in both locations than relying on the class name. Plus we have unit tests that should lessen some of our concerns here

The only other use of algorithmsSupportedForFlexibleChecksums is in a log message. I'd rather remove the log message than keep this class reflection

question: What is this used for?

It's so we can have runBlocking as a runtime type, it's used in the presigner generator

You can just add a new runBlocking type under the existing KotlinxCoroutines runtime type

correctness: missing license at the top. same applies to Concurrency.kt

Thanks, I think we need to adjust our Ktlint for be more stringent when it comes to license headers

docs: missing KDocs

Maybe we can add a new rule to Ktlint about Kdocs? Not sure it that's possible but I think I'd be helpful

correctness: Right now request and response checksum configs both use this enum. In the future, their config options might diverge, which will force us to make an API break. I'd recommend splitting this into HttpChecksumRequestConfigOption and HttpChecksumResponseConfigOption now to get ahead of that.

It seems like just speculation for now, I say we only separate them if that time comes. Changing the type of this config option would be a breaking change for all SDKs. I think it's more likely that we would make additive changes

It's not speculative, there was some discussion during the spec review about adding more options to either request or response but not both.

The spec calls out RequestChecksumCalculation and ResponseChecksumValidation as separate config types, so adding a new config option to either of those wouldn't be a breaking change for SDKs which implement it as the spec says.